REST high-level client: add simulate pipeline API #31158

sohaibiftikhar · 2018-06-06T21:17:40Z

relates to #27205

javanna · 2018-06-06T21:18:30Z

add to whitelist

elasticmachine · 2018-06-07T11:20:50Z

Pinging @elastic/es-core-infra

sohaibiftikhar · 2018-06-07T12:08:23Z

client/rest-high-level/src/main/java/org/elasticsearch/client/IngestClient.java

+     * <a href="https://www.elastic.co/guide/en/elasticsearch/reference/master/simulate-pipeline-api.html">
+     *     Simulate Pipeline API on elastic.co</a>
+     */
+    public SimulatePipelineResponse simulatePipeline(SimulatePipelineRequest request, Header... headers) throws IOException {


I see that the API has changed a bit and this is now deprecated. I will switch this to RequestOptions once a review of the other stuff has been made.

it's in, you can do so ;) can you also improve the javadocs adding the @param etc. like in the other methods?

javanna

I did a first review round and left some comments

javanna · 2018-06-08T12:29:04Z

client/rest-high-level/src/main/java/org/elasticsearch/client/IngestClient.java

+     * <a href="https://www.elastic.co/guide/en/elasticsearch/reference/master/simulate-pipeline-api.html">
+     *     Simulate Pipeline API on elastic.co</a>
+     */
+    public SimulatePipelineResponse simulatePipeline(SimulatePipelineRequest request, Header... headers) throws IOException {


it's in, you can do so ;) can you also improve the javadocs adding the @param etc. like in the other methods?

javanna · 2018-06-08T12:29:42Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

+        if (simulatePipelineRequest.getId() != null && !simulatePipelineRequest.getId().isEmpty()) {
+            builder.addPathPart(simulatePipelineRequest.getId());
+        }
+        builder.addPathPart("_simulate");


addPathPartAsIs

javanna · 2018-06-08T12:32:12Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IngestClientIT.java

+                );
+            }
+        }
+    }


I think that being this an integration test, we should randomize much less, if at all. We can have a fixed request and check that the response is expected, let's leave the randomizations for the unit tests.

can you also the test the case where you get back an error?

shall we test the failure separately?

javanna · 2018-06-08T12:34:17Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java

+        endpoint.add("_simulate");
+        assertEquals(endpoint.toString(), expectedRequest.getEndpoint());
+        assertEquals(HttpPost.METHOD_NAME, expectedRequest.getMethod());
+        assertEquals(expectedParams, expectedRequest.getParameters());


could you try it with a more realistic request body and assert on it at the end?

javanna · 2018-06-08T12:38:42Z

server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineRequest.java

@@ -126,6 +128,16 @@ public void writeTo(StreamOutput out) throws IOException {
        }
    }

+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        if (source != null) {


can the API even work when the source is null? maybe we should rather add this check to validate instead?

Looking at the constructor I don't think this will occur. Except if you create it with the blank instance constructor (which only happens in tests). I think we can remove this check if we ignore that constructor.

makes sense, the default constructor is there only for serialization, it will be used to create an instance and then call readFrom on it.

javanna · 2018-06-08T12:46:11Z

server/src/test/java/org/elasticsearch/action/ingest/SimulateProcessorResultTests.java

+
+    @Override
+    protected boolean supportsUnknownFields() {
+        return false;


same comment as above, let's return false here only if we can't insert random fields ever in any level of the object

javanna · 2018-06-08T12:46:22Z

server/src/test/java/org/elasticsearch/action/ingest/SimulateProcessorResultTests.java

+
+    @Override
+    protected boolean assertToXContentEquivalence() {
+        return false;


is this due to an exception?

Yes. Same reason as above.

cool, same comment as above :)

javanna · 2018-06-08T12:47:04Z

server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java

+        );
+    }
+
+    private boolean testSourceMapEquality(Map<String, Object> first, Map<String, Object> second) {


can we instead use ElasticsearchAssertions#assertToXContentEquivalent ?

javanna · 2018-06-08T12:47:57Z

server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java

+
+    @Override
+    protected boolean supportsUnknownFields() {
+        // Cannot support unknown fields because equality changes if new keys are added to _source


good, that makes sense

to clarify, this is because _source is mixed up with metadata right? if it was in a separate field, we would exclude it and we could return true here?

We can do that I think. Since the metadata is separate. We would also need to ignore the _ingest field.

javanna · 2018-06-08T12:52:20Z

server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java

+    @Override
+    protected WriteableIngestDocument createTestInstance() {
+        // We want to create a test instance without byte arrays. For testing byte arrays see testSourceFromXContentWithByteArray.
+        // This is needed because for comparing byte[] Object.equal fails


just an idea: could we maybe try to leave byte arrays but modify the equality assertion overriding assertEqualInstances ?

I don't know. equals and hashCode are already implemented for this so I did it like this. If you think it is better to override assertEqualInstances I will make the change.

maybe give it a try and see if it gets better. I think it may, but I would have to try as well to see what it looks like :)

sohaibiftikhar · 2018-06-14T00:47:23Z

test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java

        } else if (expected instanceof byte[]) {
            //byte[] is really a special case for binary values when comparing SMILE and CBOR, arrays of other types
            //don't need to be handled. Ordinary arrays get parsed as lists.
+            if (convertBase64Strings && actual instanceof String) {
+                // for objects deserialized from XContent the content might be a Base64 encoded string
+                actual = Base64.getDecoder().decode((String)actual);


@javanna I am not entirely sure if this is the best way to do this. The other option I can think of is to always serialize to XContent and then assert equality. But then there doesn't seem to be any point to this as XContent equality is already checked.

indeed, I am not thrilled about this part. Can you remind me why we need this? What part of WriteableIngestDocument causes issues? Also confused because you didn't replace the existing assertions but you only added new ones. I wonder if this method is not testing things properly.

So the problem is there are byte arrays in the documents (_source field). When you serialize it to XContent (if it is not CBOR) it gets serialized to a base64 string. Now when you deserialize this its hard to check for equality for these two. Since I wanted to reuse code I decided to modify it here. You are also right kind of that this method has a bug. It checks for expected instanceof byte[] and then casts actual to byte[] which can throw an exception. This is why I added the other assertion in the else clause. I think this is also true in the condition for List.

Before this change, I had separate tests for documents with byte arrays and without (similar to what we do with exceptions). Then you said maybe I should do assertEqualInstances instead of separate tests. So that led to this ;)

ok I see, what I don't follow is why we have this problem here and not in other places, as we have other API that can also hold _source in the same way. I will need to dig this up.

I got it. Look at GetResponseTests, it holds the source but most of the problems (what makes the test a bit special) is the fields part rather than source. The trick is to generate the source using RandomObjects.randomSource which doesn't add byte arrays, much easier for testing xcontent stuff. RandomDocumentPicks#randomSource is used for a lot of the extensive ingest unit tests but outside of xcontent serialization. I think if you change that you will not need all these changes.

javanna

I left a couple more comments, thanks @sohaibiftikhar ! I would not mind if also @nik9000 had a look ;)

javanna · 2018-06-14T15:01:56Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IngestClientIT.java

@@ -93,31 +90,25 @@ public void testDeletePipeline() throws IOException {
    public void testSimulatePipeline() throws IOException {
        XContentType xContentType = randomFrom(XContentType.values());
        XContentBuilder builder = XContentBuilder.builder(xContentType.xContent());
-        int numDocs = randomIntBetween(1, 10);
        boolean isVerbose = randomBoolean();


can we also have two separate tests for with and without verbose rather than randomizing it?

javanna · 2018-06-14T15:02:56Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IngestClientIT.java

+                );
+            }
+        }
+    }


shall we test the failure separately?

javanna · 2018-06-14T15:17:36Z

server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineResponse.java

+                boolean isVerbose = false;
+                boolean isFirst = true;
+                SimulateDocumentResult result = null;
+                while (p.nextToken().equals(Token.FIELD_NAME)) {


I find this loop a bit weird, probably because I am used to a different way of parsing. In a lot of other places we rather do while ( (token = parser.nextToken()) != END_OBJECT). I find that one a bit easier to read as within the while you'd then have the different ifs for the different types of tokens and then you can also reason about naming. This way of parsing also requires us to add the nextToken in the default case, which looks a bit weird too. You would usually skipChildren on a start_array or start_object.

Okay. I will try to blend this piece with the other places ;)

javanna · 2018-06-14T15:18:37Z

server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineResponse.java

+            (p, c) -> {
+                ensureExpectedToken(Token.START_OBJECT, p.currentToken(), p::getTokenLocation);
+                boolean isVerbose = false;
+                boolean isFirst = true;


maybe we can remove this variable as we only use it to assert on it? After all we are parsing our own responses and we can rely on the fact that you only get either verbose responses or ordinary ones, never both at the same time.

javanna · 2018-06-14T15:21:55Z

server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResultTests.java

@@ -57,4 +54,53 @@ public void testSerialization() throws IOException {
            assertIngestDocument(otherSimulateDocumentBaseResult.getIngestDocument(), simulateDocumentBaseResult.getIngestDocument());
        }
    }
+
+    protected SimulateDocumentBaseResult createTestInstance(boolean isFailure) {


Yes thanks!

javanna · 2018-06-14T15:22:58Z

server/src/test/java/org/elasticsearch/action/ingest/SimulatePipelineResponseTests.java

@@ -120,4 +93,104 @@ public void testSerialization() throws IOException {
            }
        }
    }
+
+    public static SimulatePipelineResponse createInstance(String pipelineId, boolean isVerbose, boolean withFailure) {


wouldn't package protected be enough?

Hmm.. probably. I will check this.

javanna · 2018-06-14T15:37:22Z

test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java

        } else if (expected instanceof byte[]) {
            //byte[] is really a special case for binary values when comparing SMILE and CBOR, arrays of other types
            //don't need to be handled. Ordinary arrays get parsed as lists.
+            if (convertBase64Strings && actual instanceof String) {
+                // for objects deserialized from XContent the content might be a Base64 encoded string
+                actual = Base64.getDecoder().decode((String)actual);


indeed, I am not thrilled about this part. Can you remind me why we need this? What part of WriteableIngestDocument causes issues? Also confused because you didn't replace the existing assertions but you only added new ones. I wonder if this method is not testing things properly.

nik9000 · 2018-06-15T14:32:35Z

server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java

+        Map<String, Object> source = IngestDocument.deepCopyMap(ingestDocument.getSourceAndMetadata());
+        metadataMap.keySet().forEach(mD -> source.remove(mD.getFieldName()));
+        builder.field(SOURCE_FIELD, source);
+        builder.field(INGEST_FIELD, ingestDocument.getIngestMetadata());


I think I like isolating this change to the parsing because it is big enough as it stands. But I think it'd be worth looking at IngestDocument in a followup.

nik9000 · 2018-06-15T14:34:34Z

server/src/test/java/org/elasticsearch/action/ingest/SimulatePipelineResponseTests.java

+        assertEquals(response.getResults().size(), parsedResponse.getResults().size());
+        for (int i=0; i < response.getResults().size(); i++) {
+            if (response.isVerbose()) {
+                assert response.getResults().get(i) instanceof SimulateDocumentVerboseResult;


I tend to use assertThat(response.getResults().get(i), instanceOf(SimulateDocumentVerboseResult.class) because it produces a nice error message. Not that it is all that important here because you can easily track down the error to the line and figure out what is up.

nik9000 · 2018-06-15T14:35:13Z

server/src/test/java/org/elasticsearch/action/ingest/SimulatePipelineResponseTests.java

+    public void testFromXContentWithFailures() throws IOException {
+        Supplier<SimulatePipelineResponse> instanceSupplier = SimulatePipelineResponseTests::createTestInstanceWithFailures;
+        //exceptions are not of the same type whenever parsed back
+        boolean assertToXContentEquivalence = false;


-- removed changed to RandomDocumentPicks

-- Started using RandomObjects instead of RandomDocumentPicks -- Removed changes to ElasticsearchAssertions

javanna

left some more comments, mostly minors, LGTM otherwise

javanna · 2018-06-19T11:28:28Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IngestClientIT.java

+    public void testSimulatePipeline() throws IOException {
+        XContentType xContentType = randomFrom(XContentType.values());
+        XContentBuilder builder = XContentBuilder.builder(xContentType.xContent());
+        boolean isVerbose = randomBoolean();


I think I have left a comment on this before, can we avoid randomizing values in integration tests and rather have two tests, one for verbose and one without verbose? Same for failures

That means four tests?

Okay. I am sorry I missed this comment from before. I separated it now.

javanna · 2018-06-19T11:35:07Z

server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResultTests.java

+    @Override
+    protected boolean assertToXContentEquivalence() {
+        return true;
+    }


Maybe we can remove this and supportsUnknownFields given that we are now using the default values?

javanna · 2018-06-19T11:36:22Z

server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResultTests.java

+
+public class SimulateDocumentBaseResultTests extends AbstractXContentTestCase<SimulateDocumentBaseResult> {
+
+    public void testSerialization() throws IOException {


nit: I wonder if transport serialization should be tested separately, in a test class that extends from AbstractStreamableTestCase

I see that you didn't add this but only moved it, then I don't mean to make this PR too big, you can leave this as-is

javanna · 2018-06-19T11:38:11Z

server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentVerboseResultTests.java

+    @Override
+    protected boolean assertToXContentEquivalence() {
+        return true;
+    }


here too , remove the methods that are no longer needed?

javanna · 2018-06-19T11:39:01Z

server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java

+
+    @Override
+    protected Predicate<String> getRandomFieldsExcludeFilter() {
+        // We cannot have random fields in the _source field and _ingest field


is it worth sharing this predicate somewhere given that it's the same in all these tests?

I was thinking the same. What would you suggest?

The same also goes for testFromXContentWithFailures. Probably should extend AbstractXContentTestCase with AbstractXContentWithFailureTests and add this method and createTestInstanceWithFailures. Then extend it everywhere.

I would keep it simple, add a static method somewhere and call it where it's needed. On extending AbstractXContentTestCase and adding another base test class, I am not too sure, just because we have already a lot of test base classes and it's hard for folks to figure out which one is right for them. Maybe it's a good idea though, if you want to go that route I am not against it, I think :)

Okay. But its not the same everywhere. At two places it is field.startsWith and the other two is field.contains. I can make everything contains and it would still work I guess. But startsWith is more accurate in some cases.

ok, I thought that they were all the same, let's not complicate it then, I am fine with leaving as-is

javanna · 2018-06-19T11:44:00Z

server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineResponse.java

+                            result = new SimulateDocumentVerboseResult(results);
+                            break;
+                        case WriteableIngestDocument.DOC_FIELD:
+                        case "error":


maybe have two cases for this rather than a single case with an if that checks again?

Yes, this made more sense before when it was a bit different. I will change this.

javanna · 2018-06-19T11:46:35Z

server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineResponse.java

+                    ensureExpectedToken(parser.currentToken(), Token.FIELD_NAME, parser::getTokenLocation);
+                    String fieldName = parser.currentName();
+                    parser.nextToken();
+                    switch(fieldName) {


I would find this even more accurate if we looked at the token type upfront rather than checking first the field name and then asserting on the token type.

But they are different for different cases?

the subtle difference is that if something with same name but wrong type comes up, now it's rejected, while checking first the token type it would be ignored. It also makes parsing code more readable because you know exactly for which type of section you do what. At least that's what I got used to.

-- Added separate tests for failure and verbose responses instead of randomization -- Changed parsing for SimulatePipelineResponse

javanna

left two super small nits. thanks @sohaibiftikhar !

javanna · 2018-06-21T09:24:47Z

server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineResponse.java

+                    token = parser.nextToken();
+                    if (token == Token.START_ARRAY) {
+                        switch (fieldName) {
+                            case SimulateDocumentVerboseResult.PROCESSOR_RESULT_FIELD:


can you do else if here please?

You mean something like:

if ( fieldName.equals(PROCESSOR_RESULT_FIELD)) { /* do something */ } else { /* skip children */ }

I wanted to make it similar to the one below so I did a switch statement. But I agree it would make more sense with an if.

yes that is what I meant, thank you!

javanna · 2018-06-21T09:26:39Z

server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineResponse.java

+                        switch (fieldName) {
+                            case SimulateDocumentVerboseResult.PROCESSOR_RESULT_FIELD:
+                                List<SimulateProcessorResult> results = new ArrayList<>();
+                                while ((token = parser.nextToken()).equals(Token.START_OBJECT)) {


nit: use == ?

javanna · 2018-06-21T14:29:11Z

retest this please

javanna · 2018-06-21T20:14:40Z

@sohaibiftikhar would you mind merging master in? Hopefully that will fix the test failures.

…o simulate_pipeline

javanna · 2018-06-22T07:59:10Z

thanks @sohaibiftikhar !

* master: Add get field mappings to High Level REST API Client (#31423) [DOCS] Updates Watcher examples for code testing (#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (#31474) [DOCS] Move monitoring to docs folder (#31477) Core: Combine doExecute methods in TransportAction (#31517) IndexShard should not return null stats (#31528) fix repository update with the same settings but different type (#31458) Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527) Node selector per client rather than per request (#31471) Core: Combine messageRecieved methods in TransportRequestHandler (#31519) Upgrade to Lucene 7.4.0. (#31529) [ML] Add ML filter update API (#31437) Allow multiple unicast host providers (#31509) Avoid deprecation warning when running the ML datafeed extractor. (#31463) REST high-level client: add simulate pipeline API (#31158) Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507) [PkiRealm] Invalidate cache on role mappings change (#31510) [Security] Check auth scheme case insensitively (#31490) In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514) [DOCS] Fix REST tests in SQL docs [DOCS] Add code snippet testing in more ML APIs (#31339) Core: Remove ThreadPool from base TransportAction (#31492) [DOCS] Remove fixed file from build.gradle Rename createNewTranslog to fileBasedRecovery (#31508) Test: Skip assertion on windows [DOCS] Creates field and document level security overview (#30937) [DOCS] Significantly improve SQL docs [DOCS] Move migration APIs to docs (#31473) Core: Convert TransportAction.execute uses to client calls (#31487) Return transport addresses from UnicastHostsProvider (#31426) Ensure local addresses aren't null (#31440) Remove unused generic type for client execute method (#31444) Introduce http and tcp server channels (#31446)

relates to #27205

* 6.x: Fix broken backport of #31578 by adjusting constructor (#31587) ingest: Add ignore_missing property to foreach filter (#22147) (#31578) Add package pre-install check for java binary (#31343) Docs: Clarify sensitive fields watcher encryption (#31551) Watcher: Remove never executed code (#31135) Improve test times for tests using `RandomObjects::addFields` (#31556) Revert "Remove RestGetAllAliasesAction (#31308)" REST high-level client: add simulate pipeline API (#31158) Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507) Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527) [Test] Add full cluster restart test for Rollup (#31533) Enhance thread context uniqueness assertion fix writeIndex evaluation for aliases (#31562) Add x-opaque-id to search slow logs (#31539) Watcher: Fix put watch action (#31524) [DOCS] Significantly improve SQL docs turn GetFieldMappingsResponse to ToXContentObject (#31544) TEST: Unmute testHistoryUUIDIsGenerated Ingest Attachment: Upgrade Tika to 1.18 (#31252) TEST: Correct the assertion arguments order (#31540)

colings86 added >enhancement :Core/Java High Level REST Client labels Jun 7, 2018

sohaibiftikhar commented Jun 7, 2018

View reviewed changes

javanna requested changes Jun 8, 2018

View reviewed changes

javanna mentioned this pull request Jun 12, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

sohaibiftikhar force-pushed the simulate_pipeline branch from 690e92a to da8fd19 Compare June 14, 2018 00:40

sohaibiftikhar commented Jun 14, 2018

View reviewed changes

javanna requested changes Jun 14, 2018

View reviewed changes

nik9000 reviewed Jun 15, 2018

View reviewed changes

sohaibiftikhar force-pushed the simulate_pipeline branch from bfcd543 to d72bb03 Compare June 18, 2018 11:08

sohaibiftikhar added 4 commits June 19, 2018 12:25

Added simulate pipeline API

e530bd4

Fixes after review

3497e0c

-- removed changed to RandomDocumentPicks

fixed integ test

1de794e

Fixes after review (2)

d08dcb5

-- Started using RandomObjects instead of RandomDocumentPicks -- Removed changes to ElasticsearchAssertions

sohaibiftikhar force-pushed the simulate_pipeline branch from d72bb03 to d08dcb5 Compare June 19, 2018 10:26

javanna approved these changes Jun 19, 2018

View reviewed changes

Fixes after review (3)

755de50

-- Added separate tests for failure and verbose responses instead of randomization -- Changed parsing for SimulatePipelineResponse

javanna approved these changes Jun 21, 2018

View reviewed changes

changed switch to if

54e7f6f

sohaibiftikhar added 2 commits June 21, 2018 22:29

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

d8f5da9

…o simulate_pipeline

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

64c9c51

…o simulate_pipeline

javanna added v7.0.0 v6.4.0 labels Jun 22, 2018

javanna merged commit eade161 into elastic:master Jun 22, 2018

javanna added the backport pending label Jun 22, 2018

sohaibiftikhar deleted the simulate_pipeline branch June 22, 2018 09:57

javanna pushed a commit that referenced this pull request Jun 26, 2018

REST high-level client: add simulate pipeline API (#31158)

adbfd1d

relates to #27205

javanna removed the backport pending label Jun 26, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		public class SimulateDocumentBaseResultTests extends AbstractXContentTestCase<SimulateDocumentBaseResult> {

		public void testSerialization() throws IOException {

REST high-level client: add simulate pipeline API #31158

REST high-level client: add simulate pipeline API #31158

Conversation

sohaibiftikhar commented Jun 6, 2018

javanna commented Jun 6, 2018

elasticmachine commented Jun 7, 2018

sohaibiftikhar Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohaibiftikhar Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

javanna Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

sohaibiftikhar Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohaibiftikhar Jun 7, 2018 •

edited

Loading

sohaibiftikhar Jun 14, 2018 •

edited

Loading

javanna Jun 14, 2018 •

edited

Loading

sohaibiftikhar Jun 14, 2018 •

edited

Loading

javanna Jun 14, 2018 •

edited

Loading